Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add operator overrides for public IComparable types, #683 #1056

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

paulirwin
Copy link
Contributor

@paulirwin paulirwin commented Dec 4, 2024

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Add operator overrides for public IComparable types

Fixes #683

Description

This is to resolve the Sonar warning about needing to override the comparison operators for any types that implement IComparable. We are only doing this here for the public types.

QualityQuery was missing Equals and GetHashCode, as well as had some possible NullReferenceExceptions, so those have been fixed as well.

@paulirwin paulirwin added the notes:improvement An enhancement to an existing feature label Dec 4, 2024
@paulirwin paulirwin requested a review from NightOwl888 December 4, 2024 05:03
@paulirwin
Copy link
Contributor Author

This change caught a NRE bug in MutableValue 😄 Fixed.

@NightOwl888
Copy link
Contributor

This is a draft PR, because I'm currently unsure about whether to override Equals and GetHashCode (and if so, how exactly to do that, i.e. what to compare) for QualityQuery. @NightOwl888 - please let me know your thoughts.

Well, being that they didn't override the default behavior (which is reference equality), a direct port would be to override each and then cascade the call to the base class.


But given the fact that the comparer uses queryID and it says "ID of this quality query" for a description, it would probably be best to use that to compare for equality/hash code. It would keep the equality and compare checks exactly in sync.

BTW - I noticed that the QualityQuery.CompareTo() implementation has an exception handler that we can remove because we can use int.TryParse() and fallback to string comparison if parsing either side returns false.

        public virtual int CompareTo(QualityQuery other)
        {
            try
            {
                // compare as ints when ids ints
                int n = int.Parse(queryID, CultureInfo.InvariantCulture);
                int nOther = int.Parse(other.queryID, CultureInfo.InvariantCulture);
                return n - nOther;
            }
            catch (Exception e) when (e.IsNumberFormatException())
            {
                // fall back to string comparison
                return queryID.CompareToOrdinal(other.queryID);
            }
        }

The constructor could also be improved by using guard clauses on queryID and nameValPairs since there will be NREs if either is passed null. If we do so, we don't need to change CompareTo() to account for the fact that queryID may be null (which it currently doesn't handle).

#nullable enable would catch this sort of thing.

@paulirwin paulirwin marked this pull request as ready for review December 4, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:improvement An enhancement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When implementing IComparable, you should also override ==, !=, <, <=, >, and >=
2 participants